Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow vanilla (de)serialization of discriminated unions #132

Merged
merged 3 commits into from
Aug 13, 2024
Merged

Conversation

DiamondJoseph
Copy link
Contributor

@DiamondJoseph DiamondJoseph commented Aug 8, 2024

Fixes #130, #131

@DiamondJoseph DiamondJoseph force-pushed the bug-fix branch 4 times, most recently from dbc2615 to 5d9ff6b Compare August 9, 2024 09:59
Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.25%. Comparing base (f49320c) to head (688265e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
- Coverage   96.31%   96.25%   -0.06%     
==========================================
  Files           9        9              
  Lines         949      935      -14     
==========================================
- Hits          914      900      -14     
  Misses         35       35              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DiamondJoseph DiamondJoseph changed the title initial commit Allow vanilla serialisation/deserialisation of discriminated unions Aug 9, 2024
@DiamondJoseph DiamondJoseph force-pushed the bug-fix branch 4 times, most recently from 0fa2ccf to 57f627e Compare August 9, 2024 14:11
@DiamondJoseph DiamondJoseph marked this pull request as ready for review August 9, 2024 14:16
@DiamondJoseph
Copy link
Contributor Author

DiamondJoseph commented Aug 9, 2024

Current handling requires that subclasses of type annotated with discriminated_union_of_subclasses are Pydantic dataclasses, which I will add to the docstring: this way their schema is generated after Spec has had it schema updated.

Types that want to have their schema updated when a new instance of a class decorated with discriminated_union_of_subclasses must also either be Pydantic dataclasses or BaseModels.

The FastAPI schema for the routes into the service does not update when a new subclass is created, nor do any types that refer to types that refer to a discriminated union.

If Pydantic exposed some way of propagating that a schema that is used in the schema of a model has updated, so the schema of the model must also update, then _TaggedUnion could remove all handling of references and just rebuild_dataclass(self._baseclass)

@DiamondJoseph
Copy link
Contributor Author

I've made an issue for that last part, but I don't think it will gain much traction as I think creating runtime models often goes against the purpose of validation

pydantic/pydantic#10091

@DiamondJoseph DiamondJoseph changed the title Allow vanilla serialisation/deserialisation of discriminated unions Allow vanilla(de)serialization of discriminated unions Aug 9, 2024
@DiamondJoseph DiamondJoseph changed the title Allow vanilla(de)serialization of discriminated unions Allow vanilla (de)serialization of discriminated unions Aug 9, 2024
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, minor points only, thanks for this!

src/scanspec/core.py Show resolved Hide resolved
src/scanspec/core.py Outdated Show resolved Hide resolved
src/scanspec/core.py Outdated Show resolved Hide resolved
src/scanspec/core.py Show resolved Hide resolved
@@ -104,134 +102,106 @@ def calculate(self) -> int:
)

Args:
super_cls: The superclass of the union, Expression in the above example
cls: The superclass of the union, Expression in the above example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cls: The superclass of the union, Expression in the above example
super_cls: The superclass of the union, Expression in the above example

Copy link
Contributor

@ZohebShaikh ZohebShaikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you Joseph

@DiamondJoseph DiamondJoseph merged commit d5cce50 into main Aug 13, 2024
19 of 20 checks passed
@DiamondJoseph DiamondJoseph deleted the bug-fix branch August 13, 2024 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't have a Spec as a field in another model
3 participants